Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not parenthesize exterior struct lit inside match guards #118726

Merged
merged 7 commits into from
Dec 11, 2023

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Dec 7, 2023

Before this PR, the AST pretty-printer injects parentheses around expressions any time parens could be needed depending on what else is in the code that surrounds that expression. But the pretty-printer did not pass around enough context to understand whether parentheses really are needed on any particular expression. As a consequence, there are false positives where unneeded parentheses are being inserted.

Example:

#![feature(if_let_guard)]

macro_rules! pp {
    ($e:expr) => {
        stringify!($e)
    };
}

fn main() {
    println!("{}", pp!(match () { () if let _ = Struct {} => {} }));
}

Before:

match () { () if let _ = (Struct {}) => {} }

After:

match () { () if let _ = Struct {} => {} }

This PR introduces a bit of state that is passed across various expression printing methods to help understand accurately whether particular situations require parentheses injected by the pretty printer, and it fixes one such false positive involving match guards as shown above.

There are other parenthesization false positive cases not fixed by this PR. I intend to address these in follow-up PRs. For example here is one: the expression { let _ = match x {} + 1; } is pretty-printed as { let _ = (match x {}) + 1; } despite there being no reason for parentheses to appear there.

@rustbot
Copy link
Collaborator

rustbot commented Dec 7, 2023

r? @compiler-errors

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 7, 2023
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Dec 8, 2023

☔ The latest upstream changes (presumably #118527) made this pull request unmergeable. Please resolve the merge conflicts.

True || needs_par_as_let_scrutinee(...) is always true.
In all four of Break, Closure, Ret, Yeet, the needs_par_as_let_scrutinee
is guaranteed to return true because the .precedence().order() of those
expr kinds is <= AssocOp::LAnd.precedence().

The relevant functions in rustc_ast::util::parser are:

    fn needs_par_as_let_scrutinee(order: i8) -> bool {
        order <= prec_let_scrutinee_needs_par() as i8
    }

    fn prec_let_scrutinee_needs_par() -> usize {
        AssocOp::LAnd.precedence()
    }

The .precedence().order() of Closure is PREC_CLOSURE (-40) and of Break,
Ret, Yeet is PREC_JUMP (-30).

The value of AssocOp::LAnd.precedence() is 6.

So this commit causes no change in behavior, only potentially
performance by doing a redundant call to contains_exterior_struct_lit in
those four cases. This is fine because Break, Closure, Ret, Yeet should
be exceedingly rare in the position of a let scrutinee.
    tidy error: /git/rust/compiler/rustc_ast_pretty/src/pprust/state.rs:1165: unexplained "```ignore" doctest; try one:

    * make the test actually pass, by adding necessary imports and declarations, or
    * use "```text", if the code is not Rust code, or
    * use "```compile_fail,Ennnn", if the code is expected to fail at compile time, or
    * use "```should_panic", if the code is expected to fail at run time, or
    * use "```no_run", if the code should type-check but not necessary linkable/runnable, or
    * explain it like "```ignore (cannot-test-this-because-xxxx)", if the annotation cannot be avoided.

    tidy error: /git/rust/compiler/rustc_ast_pretty/src/pprust/state.rs:1176: unexplained "```ignore" doctest; try one:

    * make the test actually pass, by adding necessary imports and declarations, or
    * use "```text", if the code is not Rust code, or
    * use "```compile_fail,Ennnn", if the code is expected to fail at compile time, or
    * use "```should_panic", if the code is expected to fail at run time, or
    * use "```no_run", if the code should type-check but not necessary linkable/runnable, or
    * explain it like "```ignore (cannot-test-this-because-xxxx)", if the annotation cannot be avoided.
@dtolnay
Copy link
Member Author

dtolnay commented Dec 8, 2023

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thanks

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 11, 2023

📌 Commit 8997215 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 11, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 11, 2023
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#118620 (resolve: Use `def_kind` query to cleanup some code)
 - rust-lang#118647 (dump bootstrap shims)
 - rust-lang#118726 (Do not parenthesize exterior struct lit inside match guards)
 - rust-lang#118818 (llvm-wrapper: adapt for LLVM API change)
 - rust-lang#118822 (Extract exhaustiveness into its own crate)
 - rust-lang#118826 (Edit target doc template to remove email)
 - rust-lang#118827 (Update table for linker-plugin-lto docs)
 - rust-lang#118835 (Fix again `rustc_codegen_gcc` tests)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3a0562b into rust-lang:master Dec 11, 2023
11 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 11, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 11, 2023
Rollup merge of rust-lang#118726 - dtolnay:matchguardlet, r=compiler-errors

Do not parenthesize exterior struct lit inside match guards

Before this PR, the AST pretty-printer injects parentheses around expressions any time parens _could_ be needed depending on what else is in the code that surrounds that expression. But the pretty-printer did not pass around enough context to understand whether parentheses really _are_ needed on any particular expression. As a consequence, there are false positives where unneeded parentheses are being inserted.

Example:

```rust
#![feature(if_let_guard)]

macro_rules! pp {
    ($e:expr) => {
        stringify!($e)
    };
}

fn main() {
    println!("{}", pp!(match () { () if let _ = Struct {} => {} }));
}
```

**Before:**

```console
match () { () if let _ = (Struct {}) => {} }
```

**After:**

```console
match () { () if let _ = Struct {} => {} }
```

This PR introduces a bit of state that is passed across various expression printing methods to help understand accurately whether particular situations require parentheses injected by the pretty printer, and it fixes one such false positive involving match guards as shown above.

There are other parenthesization false positive cases not fixed by this PR. I intend to address these in follow-up PRs. For example here is one: the expression `{ let _ = match x {} + 1; }` is pretty-printed as `{ let _ = (match x {}) + 1; }` despite there being no reason for parentheses to appear there.
@dtolnay dtolnay deleted the matchguardlet branch December 11, 2023 23:21
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 27, 2023
Fix parenthesization of subexprs containing statement boundary

This PR fixes a multitude of false negatives and false positives in the AST pretty printer's parenthesis insertion related to statement boundaries &mdash; statements which terminate unexpectedly early if there aren't parentheses.

Without this fix, the AST pretty printer (including both `stringify!` and `rustc -Zunpretty=expanded`) is prone to producing output which is not syntactically valid Rust. Invalid output is problematic because it means Rustfmt is unable to parse the output of `cargo expand`, for example, causing friction by forcing someone trying to debug a macro into reading poorly formatted code.

I believe the set of bugs fixed in this PR account for the most prevalent reason that `cargo expand` produces invalid output in real-world usage.

Fixes rust-lang#98790.

## False negatives

The following is a correct program &mdash; `cargo check` succeeds.

```rust
macro_rules! m {
    ($e:expr) => {
        match () { _ => $e }
    };
}

fn main() {
    m!({ 1 } - 1);
}
```

But `rustc -Zunpretty=expanded main.rs` produces output that is invalid Rust syntax, because parenthesization is needed and not being done by the pretty printer.

```rust
fn main() { match () { _ => { 1 } - 1, }; }
```

Piping this expanded code to rustfmt, it fails to parse.

```console
error: unexpected `,` in pattern
 --> <stdin>:1:38
  |
1 | fn main() { match () { _ => { 1 } - 1, }; }
  |                                      ^
  |
help: try adding parentheses to match on a tuple...
  |
1 | fn main() { match () { _ => { 1 } (- 1,) }; }
  |                                   +    +
help: ...or a vertical bar to match on multiple alternatives
  |
1 | fn main() { match () { _ => { 1 } - 1 | }; }
  |                                   ~~~~~
```

Fixed output after this PR:

```rust
fn main() { match () { _ => ({ 1 }) - 1, }; }
```

## False positives

Less problematic, but worth fixing (just like rust-lang#118726).

```rust
fn main() {
    let _ = match () { _ => 1 } - 1;
}
```

Output of `rustc -Zunpretty=expanded lib.rs` before this PR. There is no reason parentheses would need to be inserted there.

```rust
fn main() { let _ = (match () { _ => 1, }) - 1; }
```

After this PR:

```rust
fn main() { let _ = match () { _ => 1, } - 1; }
```

## Alternatives considered

In this PR I opted to parenthesize only the leading subexpression causing the statement boundary, rather than the entire statement. Example:

```rust
macro_rules! m {
    ($e:expr) => {
        $e
    };
}

fn main() {
    m!(loop { break [1]; }[0] - 1);
}
```

This PR produces the following pretty-printed contents for fn main:

```rust
(loop { break [1]; })[0] - 1;
```

A different equally correct output would be:

```rust
(loop { break [1]; }[0] - 1);
```

I chose the one I did because it is the *only* approach used by handwritten code in the standard library and compiler. There are 4 places where parenthesization is being used to prevent a statement boundary, and in all 4, the developer has chosen to parenthesize the smallest subexpression rather than the whole statement:

https://github.com/rust-lang/rust/blob/b37d43efd9c5a7a3d76ed21c454dd0f40945d77d/compiler/rustc_codegen_cranelift/example/alloc_system.rs#L102

https://github.com/rust-lang/rust/blob/b37d43efd9c5a7a3d76ed21c454dd0f40945d77d/compiler/rustc_parse/src/errors.rs#L1021-L1029

https://github.com/rust-lang/rust/blob/b37d43efd9c5a7a3d76ed21c454dd0f40945d77d/library/core/src/future/poll_fn.rs#L151

https://github.com/rust-lang/rust/blob/b37d43efd9c5a7a3d76ed21c454dd0f40945d77d/library/core/src/ops/range.rs#L824-L828
@dtolnay dtolnay added the A-pretty Area: Pretty printing (including `-Z unpretty`) label Dec 28, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request May 12, 2024
Fix, document, and test parser and pretty-printer edge cases related to braced macro calls

_Review note: this is a deceptively small PR because it comes with 145 lines of docs and 196 lines of tests, and only 25 lines of compiler code changed. However, I recommend reviewing it 1 commit at a time because much of the effect of the code changes is non-local i.e. affecting code that is not visible in the final state of the PR. I have paid attention that reviewing the PR one commit at a time is as easy as I can make it. All of the code you need to know about is touched in those commits, even if some of those changes disappear by the end of the stack._

This is a follow-up to rust-lang#119105. One case that is not relevant to `-Zunpretty=expanded`, but which came up as I'm porting rust-lang#119105 and rust-lang#118726 into `syn`'s printer and `prettyplease`'s printer where it **is** relevant, and is also relevant to rustc's `stringify!`, is statement boundaries in the vicinity of braced macro calls.

Rustc's AST pretty-printer produces invalid syntax for statements that begin with a braced macro call:

```rust
macro_rules! stringify_item {
    ($i:item) => {
        stringify!($i)
    };
}

macro_rules! repro {
    ($e:expr) => {
        stringify_item!(fn main() { $e + 1; })
    };
}

fn main() {
    println!("{}", repro!(m! {}));
}
```

**Before this PR:** output is not valid Rust syntax.

```console
fn main() { m! {} + 1; }
```

```console
error: leading `+` is not supported
 --> <anon>:1:19
  |
1 | fn main() { m! {} + 1; }
  |                   ^ unexpected `+`
  |
help: try removing the `+`
  |
1 - fn main() { m! {} + 1; }
1 + fn main() { m! {}  1; }
  |
```

**After this PR:** valid syntax.

```console
fn main() { (m! {}) + 1; }
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-pretty Area: Pretty printing (including `-Z unpretty`) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants